If SyncFilesToFilestore returns an error (e.g., due to a transient download failure of a single file), any partial results (such as successfully stored files and newly discovered inaccessible items) are currently discarded because the controller returns early.

To make the status updates more resilient and ensure inaccessible items are surfaced even when some errors occur, we should update the status with the partial results before returning the error. Additionally, we should add a defensive nil check on syncResult to prevent potential nil pointer dereferences.

In S3BucketSource.SyncFilesToFilestore, when len(errorMessage) > 0 (at line 120), the method currently returns nil, errors.New(errorMessage).

To ensure consistency with GDriveSource and to allow the controller to safely update the status with successfully processed files even when some errors occur, please update line 120 to return the partial SyncResult instead of nil:

return &SyncResult{StoredFiles: storedFiles}, errors.New(errorMessage)

For list fields in Kubernetes CRDs, it is highly recommended to explicitly define +listType and +listMapKey markers. This ensures correct merging behavior during Server-Side Apply (SSA) and prevents duplicate keys.

Please update the struct as follows:

type InaccessibleItems struct {
	// +listType=map
	// +listMapKey=folderID
	Folders               []InaccessibleFolder         `json:"folders,omitempty"`
	// +listType=map
	// +listMapKey=fileID
	Files                 []InaccessibleFile           `json:"files,omitempty"`
	// +listType=map
	// +listMapKey=shortcutFileID
	ShortcutTargetFolders []InaccessibleShortcutFolder `json:"shortcutTargetFolders,omitempty"`
	// +listType=map
	// +listMapKey=shortcutFileID
	ShortcutTargetFiles   []InaccessibleShortcutFile   `json:"shortcutTargetFiles,omitempty"`
}

Pre-allocating the slices in operatorv1alpha1.InaccessibleItems using make with the appropriate capacity avoids multiple allocations and slice copies during the append loops.

	result := &operatorv1alpha1.InaccessibleItems{
		Folders:               make([]operatorv1alpha1.InaccessibleFolder, 0, len(items.Folders)),
		Files:                 make([]operatorv1alpha1.InaccessibleFile, 0, len(items.Files)),
		ShortcutTargetFolders: make([]operatorv1alpha1.InaccessibleShortcutFolder, 0, len(items.ShortcutTargetFolders)),
		ShortcutTargetFiles:   make([]operatorv1alpha1.InaccessibleShortcutFile, 0, len(items.ShortcutTargetFiles)),
	}
	for _, f := range items.Folders {
		result.Folders = append(result.Folders, operatorv1alpha1.InaccessibleFolder{
			FolderID: f.FolderID, FolderName: f.FolderName, RootFolderID: f.RootFolderID,
		})
	}
	for _, f := range items.Files {
		result.Files = append(result.Files, operatorv1alpha1.InaccessibleFile{
			FileID: f.FileID, ParentFolderID: f.ParentFolderID, RootFolderID: f.RootFolderID,
		})
	}
	for _, f := range items.ShortcutTargetFolders {
		result.ShortcutTargetFolders = append(result.ShortcutTargetFolders, operatorv1alpha1.InaccessibleShortcutFolder{
			ShortcutFileID: f.ShortcutFileID, TargetFolderID: f.TargetFolderID, RootFolderID: f.RootFolderID,
		})
	}
	for _, f := range items.ShortcutTargetFiles {
		result.ShortcutTargetFiles = append(result.ShortcutTargetFiles, operatorv1alpha1.InaccessibleShortcutFile{
			ShortcutFileID: f.ShortcutFileID, TargetFileID: f.TargetFileID, RootFolderID: f.RootFolderID,
		})
	}

If SyncFilesToFilestore returns an error (e.g., due to transient download failures of some files in Phase 2), the controller immediately returns and calls r.handleError. This completely discards any successfully processed files and, more importantly, the list of inaccessible items discovered during the crawl phase (Phase 1). We should update the status with the partial results even if there's an error, as long as syncResult is not nil.

The total number of inaccessible items is calculated redundantly here. Since toAPIInaccessibleItems already calculates total to check if it's 0, we can return this count from toAPIInaccessibleItems to avoid recalculating it in Reconcile.

	inaccessible, total := toAPIInaccessibleItems(syncResult.InaccessibleItems)
	if inaccessible != nil {
		successMessage += fmt.Sprintf(", %d inaccessible items (service account may lack access)", total)
	}

Modify toAPIInaccessibleItems to return both the mapped inaccessible items and the total count. This avoids redundant calculations in the reconciler.

func toAPIInaccessibleItems(items gdrive.InaccessibleItems) (*operatorv1alpha1.InaccessibleItems, int) {
	total := len(items.Folders) + len(items.Files) + len(items.ShortcutTargetFolders) + len(items.ShortcutTargetFiles)
	if total == 0 {
		return nil, 0
	}
	result := &operatorv1alpha1.InaccessibleItems{}
	for _, f := range items.Folders {
		result.Folders = append(result.Folders, operatorv1alpha1.InaccessibleFolder{
			FolderID: f.FolderID, FolderName: f.FolderName, RootFolderID: f.RootFolderID,
		})
	}
	for _, f := range items.Files {
		result.Files = append(result.Files, operatorv1alpha1.InaccessibleFile{
			FileID: f.FileID, ParentFolderID: f.ParentFolderID, RootFolderID: f.RootFolderID,
		})
	}
	for _, f := range items.ShortcutTargetFolders {
		result.ShortcutTargetFolders = append(result.ShortcutTargetFolders, operatorv1alpha1.InaccessibleShortcutFolder{
			ShortcutFileID: f.ShortcutFileID, TargetFolderID: f.TargetFolderID, RootFolderID: f.RootFolderID,
		})
	}
	for _, f := range items.ShortcutTargetFiles {
		result.ShortcutTargetFiles = append(result.ShortcutTargetFiles, operatorv1alpha1.InaccessibleShortcutFile{
			ShortcutFileID: f.ShortcutFileID, TargetFileID: f.TargetFileID, RootFolderID: f.RootFolderID,
		})
	}
	return result, total
}

Can we have a better function name ?

Will this work if the source is s3 ?

Are we updating the status twice here ?

agreed, we are writing error twice, @gshikhar2021, can you please fix this?

@gshikhar2021 can we address this comment please, as this will hit the kube api server redundantly

We need to have some limit here, because if there are let's say for example 1000 folders then the CR size would be huge and it would be restricted by k8s

instead of defining 4 different types, can we do in one?

type InaccessibleItem struct {
    Kind         string `json:"kind"`                   // folder, file, shortcutFolder, shortcutFile
    ID       string `json:"itemID"`
    Name         string `json:"name,omitempty"`
    TargetID     string `json:"targetID,omitempty"`      // for shortcuts
    RootFolderID string `json:"rootFolderID"`
    Reason string `json:"reason"`
}

also included a reason field which I think will be helpful. In case of gdrive, we can get the information from GetFileMetadata/ListFolderContents

this can then be:

we might have to see what's the upper cap that CR can hold else we might end up blowing 1.5MB limit

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include the inaccessible-item count in failure conditions.

The error path discards total (inaccessible, _) and calls UpdateStatus("", syncErr), while only the success path appends the count. Partial failures therefore publish the summary but omit its count from the reconciliation condition message. Reuse total when constructing the error condition message.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/sourcecrawler_controller.go` around lines 149 - 156, The
failure path in the sync status update discards the inaccessible-item total and
omits it from the condition message. In the syncErr handling block, retain the
total returned by buildInaccessibleSummary and pass a message to
sourceCrawlerCR.UpdateStatus that includes that count, matching the success-path
condition format.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Avoid the second status patch on partial sync errors.

When syncResult != nil and syncErr != nil, Lines 152-160 patch partial status, then Line 162 calls handleError, which issues another status patch. Combine the partial fields and failing condition in one StatusPatch, then return the error after that patch; this is the same redundant-write issue previously raised.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/sourcecrawler_controller.go` around lines 149 - 162,
Update the sync-error path in the controller reconciliation flow so the partial
status fields and failing condition are applied in a single StatusPatch when
syncResult is non-nil. Avoid calling handleError after that patch, and return
the sync error directly while preserving existing patch-error handling.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Bound inaccessible root-folder status details.

maxInaccessibleItems caps only item details; buildInaccessibleRootFolders copies every failed root and its unbounded error string into status. Many inaccessible roots can therefore make the status patch exceed Kubernetes object-size limits. Cap this list too and preserve the uncapped total in the condition/message, or enforce equivalent input limits.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/sourcecrawler_controller.go` around lines 342 - 358,
Update buildInaccessibleRootFolders to cap the returned InaccessibleRootFolder
entries using the existing maxInaccessibleItems limit, including only the
allowed prefix while preserving the total failed-root count for the
condition/message that reports status. Ensure oversized Error strings or
equivalent unbounded root-folder details cannot bypass the status-size bound.